-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: --debug.etherscan for fake consensus client #8082
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, left some suggestions
crates/consensus/rpc/src/lib.rs
Outdated
let block: EtherscanBlockResponse = self | ||
.http_client | ||
.get(&self.etherscan_base_api_url) | ||
.query(&[ | ||
("module", "proxy"), | ||
("action", "eth_getBlockByNumber"), | ||
("tag", "latest"), | ||
("boolean", "true"), | ||
("apikey", &self.etherscan_api_key), | ||
]) | ||
.send() | ||
.await | ||
.unwrap() | ||
.json() | ||
.await |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to make this configurable, for example I think it could be useful to provide another node's rpc endpoint and use eth_subscribe + eth_getBlockByHash to fetch the block
so perhaps a stream type helper that yields full blocks
this is perhaps a better solution that just etherscan,
see for example:
crates/consensus/rpc/src/lib.rs
Outdated
reth_rpc_types::engine::ForkchoiceState { | ||
head_block_hash: block_hash, | ||
safe_block_hash: block_hash, | ||
finalized_block_hash: block_hash, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might break certain assumptions about blocks that can (or cannot in this case) be reorged. i'd prefer that API call above was a batch call that also fetches safe & finalized hashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good callout. I haven't found a way to get safe/finalized blocks state from Etherscan, because their eth_getBlockByNumber
doesn't support tags other than latest
. maybe you know some tricks? afaik beaconscan isn't exposed in API.
otherwise, should I just change the --debug.etherscan
parameter to --debug.consensus-rpc
and use its eth_getBlockByNumber
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safe block hash will be at most 32 blocks behind head, and finalized is at most 32 behind that, so you could use head, head-32, head-64
after fetching the three hashes
/// by number to fetch past finalized and safe blocks. | ||
pub trait BlockProvider: Send + Sync + 'static { | ||
/// Spawn a block provider to send new blocks to the given sender. | ||
fn spawn(&self, tx: mpsc::Sender<RichBlock>) -> impl Future<Output = ()> + Send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer, this or impl Stream<Output = RichBlock>
? IMO impl Stream<Output = RichBlock>
is much less nicer to implement (bc of manual state machine building vs just async fn
), but can change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I'd prefer to see this generalised to any node infrastructure service that uses API keys, not only Etherscan. wdyt @shekhirin @mattsse @rkrasiuk ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should work,
one question and nit,
need to think about the trait a bit more, but I see why the spawn is preferable.
// Load previous block hashes. We're using (head - 32) and (head - 64) as the safe and | ||
// finalized block hashes. | ||
// todo: chdeck for off by ones in offset | ||
let safe_block_hash = get_or_fetch_previous_block( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a function of self
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, if understood this comment correctly. if not — ping again pls.
@mattsse highlighting this PR just in case it slipped through the cracks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great!
I made some smol edits re naming, derives, ptal.
and only have a few more pedantic nits re the cli args
…pc-consensus # Conflicts: # Cargo.toml
…pc-consensus # Conflicts: # crates/node/builder/Cargo.toml
@mattsse thank you for review, updated! |
…pc-consensus # Conflicts: # Cargo.toml
…pc-consensus # Conflicts: # Cargo.lock
@mattsse just following up here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
works as expected, there are a few things we can do as followups, like adding support for v4 endpoints, but not urgend atm, will track separately
--debug.etherscan [<ETHERSCAN_API_URL>] | ||
Runs a fake consensus client that advances the chain using recent block hashes on Etherscan. If specified, requires an `ETHERSCAN_API_KEY` environment variable | ||
|
||
--debug.rpc-consensus-ws <RPC_CONSENSUS_WS> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limiting this to ws is reasonable, no need to also support HTTP polling
This is an early draft for fake consensus client that fetches blocks from Etherscan and propagates it to execution layer as FCUs and new payloads. Designed to replace
--debug.tip
when developing. #6711